Skip to content

Improved docs, package.json and .npmignore for package preparation#61

Open
Mehulantony wants to merge 1 commit intomainfrom
mehul/npm-package-docs
Open

Improved docs, package.json and .npmignore for package preparation#61
Mehulantony wants to merge 1 commit intomainfrom
mehul/npm-package-docs

Conversation

@Mehulantony
Copy link
Copy Markdown
Collaborator

Added npm installation instructions, clarified environment variable security, and documented packaging verification steps (npm pack --dry-run, npm publish --dry-run) to ensure safe and clean npm publishing.

Copy link
Copy Markdown
Collaborator

@thehabes thehabes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lack knowledge in this area so I don't want to approve or request changes. I relied heavily on the static reviewer. Look over the comments and see if they have any merit and warrant changes.

Static Review Comments

Branch: mehul/npm-package-docs
Review Date: 2026-03-30
Reviewer: Pair Static Review - Claude & @thehabes

Claude and Bryan make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.

Category Issues Found
🔴 Critical 1
🟠 Major 1
🟡 Minor 1
🔵 Suggestions 0

Critical Issues 🔴

Issue 1: Missing critical runtime files in files array — package will crash on import

File: package.json:32-43
Category: Breaking Change / Runtime Error

Problem:
The files array lists the directories and files to include in the npm tarball, but omits three root-level modules that are required by the included code:

Missing File Imported By
utils.js controllers/utils.js, controllers/crud.js, controllers/history.js, controllers/bulk.js, controllers/putUpdate.js, controllers/patchUpdate.js, controllers/patchSet.js, controllers/patchUnset.js, controllers/overwrite.js, controllers/release.js, controllers/search.js, controllers/gog.js, controllers/delete.js
rest.js app.js, routes/patchUpdate.js, routes/patchUnset.js, routes/patchSet.js
db-controller.js Nearly every route handler (routes/create.js, routes/query.js, routes/delete.js, routes/id.js, routes/history.js, routes/since.js, routes/search.js, routes/release.js, routes/overwrite.js, routes/putUpdate.js, routes/patchUpdate.js, routes/patchSet.js, routes/patchUnset.js, routes/bulkCreate.js, routes/bulkUpdate.js, routes/client.js, GOG routes)

Confirmed via npm pack --dry-run — none of these three files appear in the tarball. Any consumer running import { app } from 'rerum_server_nodejs' will get a MODULE_NOT_FOUND error.

Current Code:

  "files": [
    "app.js",
    "index.js",
    "bin/",
    "auth/",
    "config/",
    "controllers/",
    "database/",
    "public/",
    "routes/",
    "README.md"
  ],

Suggested Fix:

  "files": [
    "app.js",
    "index.js",
    "utils.js",
    "rest.js",
    "db-controller.js",
    "bin/",
    "auth/",
    "config/",
    "controllers/",
    "database/",
    "public/",
    "routes/",
    "README.md"
  ],

How to Verify:

npm pack --dry-run 2>&1 | grep -E "(utils\.js|rest\.js|db-controller\.js)" | grep -v controllers/

You should see all three root-level files listed. Then install locally and verify import works:

npm pack && mkdir /tmp/test-rerum && cd /tmp/test-rerum && npm init -y && npm install /path/to/rerum_server_nodejs-0.0.0.tgz && node -e "import('rerum_server_nodejs')"

Major Issues 🟠

Issue 1: Test and mock files leak into the published package

File: package.json:32-43
Category: Code Hygiene / Package Size

Problem:
22 test and mock files are included in the tarball (~82 KB of unnecessary content). The .npmignore already has patterns for __tests__/ and __mocks__/, but per npm docs: a root-level .npmignore does not override the files field. Since files includes whole directories like routes/, auth/, and database/, test subdirectories within them are packaged regardless of .npmignore.

Confirmed via npm pack --dry-run:

auth/__mocks__/index.txt
auth/__tests__/token.test.txt
database/__mocks__/index.txt
routes/__tests__/bulkCreate.test.js
routes/__tests__/create.test.js
... (22 files total)

Consumers of the package don't need test files. This inflates package size and may confuse users browsing node_modules.

Suggested Fix — Since .npmignore cannot override files, use negation globs directly in the files array:

  "files": [
    "app.js",
    "index.js",
    "utils.js",
    "rest.js",
    "db-controller.js",
    "bin/",
    "auth/",
    "config/",
    "controllers/",
    "database/",
    "public/",
    "routes/",
    "README.md",
    "!**/__tests__/",
    "!**/__mocks__/"
  ],

If negation globs in files don't work for your npm version, the alternative is to list specific subdirectories instead of whole parent directories (e.g. routes/*.js instead of routes/).

How to Verify:

npm pack --dry-run 2>&1 | grep -E "(__tests__|__mocks__|\.test\.)"

Should return no results.

Minor Issues 🟡

Issue 1: files array has inconsistent indentation

File: package.json:32
Category: Code Hygiene / JSON Formatting

Problem:
The files key has extra leading whitespace compared to sibling keys. While valid JSON, it breaks the formatting consistency of the file, making it look like files is nested under main when it's actually a peer.

Current Code:

  "main": "index.js",
    "files": [
    "app.js",

Suggested Fix:

  "main": "index.js",
  "files": [
    "app.js",

How to Verify:
Visual inspection. Run a JSON formatter to confirm the file parses correctly and is consistently indented.


Suggestions 🔵

None.


If there are significant code changes in response to this review please test those changes. Run the application manually and test or perform internal application tests when applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants